-
Notifications
You must be signed in to change notification settings - Fork 298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: convert ava tests to mocha #975
Conversation
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@typescript-eslint/[email protected], npm/[email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on! Glad you were able to keep these pretty similar while dropping the ava
dependency.
But that actually makes me wonder how @Shaptic feels about them. I think it's a little bit silly that we have the tests/e2e
directory at all, or a separate script in scripts
to run them. Are these really end to end tests? Maybe we could just name them something more accurate and I'd be happy with them.
Which kind of leads to larger questions.
- Do we like the other directories, inside
test
?unit
seems to contain a lot of tests that aren't, strictly-speaking, unit tests- this new
e2e
stuff is sort of integration-type tests, but theintegration
directory doesn't look similar enough to make sense to re-use this directory.
- only one file within
unit
authors the test itself in TypeScript. Should all the tests be authored in TypeScript? (Note that this will lead to some (surmountable) typing challenges with thesee2e
tests, sinceAssembledTransaction
is generic and would need to be explicitly typed (or explicitly ignored) in the TS files.)
Maybe we don't have to answer all of these questions now. However we want to evolve this codebase, having fewer dependencies and testing tools is a good thing!
Aside from those higher-level questions, I noticed some small things we can improve below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow you rock for powering through on this 👏
@chadoh I definitely agree that the testing is a little messy and I don't have great answers for your questions at the moment (especially e2e
since I don't even know how those work, strictly speaking), but this PR is definitely a great move in the right direction in terms of cleaning them all up 👏
f7b853d
to
8bed9f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
8bed9f4
to
793aab3
Compare
Signed-off-by: blaineheffron <[email protected]>
remove line Co-authored-by: Chad Ostrowski <[email protected]> Signed-off-by: blaineheffron <[email protected]>
Signed-off-by: blaineheffron <[email protected]>
Signed-off-by: blaineheffron <[email protected]>
Signed-off-by: blaineheffron <[email protected]>
more precise wording Co-authored-by: Chad Ostrowski <[email protected]> Signed-off-by: blaineheffron <[email protected]>
Signed-off-by: blaineheffron <[email protected]>
Signed-off-by: blaineheffron <[email protected]>
clean up nested awaits Co-authored-by: Chad Ostrowski <[email protected]> Signed-off-by: blaineheffron <[email protected]>
793aab3
to
79aaf19
Compare
Converts the e2e tests to using mocha. Removes the ava dependency.